Support write and read of boolean masks in `wcs1d-fits` #1051

dhomeier · 2023-03-29T15:22:42Z

This PR includes a test that was missed before merging #1009 and adds functionality to write Spectrum1D masks of dtype = bool in WCS1D format, resolving an issue that came up in #1009 (comment).
As bool or bit cannot be written natively to FITS, enabling round-tripping requires encoding the original type, for which there also seems to be no standard. I propose to introduce the header keyword 'BFORM', which is neither defined nor reserved in the FITS standard, and use it analogously to 'TFORMn' in BINTABLE extensions, for which these values are allowed (excerpt):

TFORMn value Description
’L’ Logical
’X’ Bit
’B’ Unsigned byte
’I’ 16-bit integer

Here BFORM is only used for the 1-bit types, as other formats are already recognised, but it might be added for others for consistency.

dhomeier · 2023-03-29T17:12:40Z

@rosteen do PRs need extra approval for the CI?

rosteen · 2023-03-29T17:14:43Z

@rosteen do PRs need extra approval for the CI?

They shouldn't, since you've contributed here before.

pllim

LGTM. Thanks!

pllim · 2023-03-29T17:16:04Z

Actions was having problem. I am going to try to close/re-open.

pllim · 2023-03-29T17:16:56Z

If CI does not kick off still, maybe just wait out the server problems. https://www.githubstatus.com/

dhomeier · 2023-03-29T17:19:13Z

They shouldn't, since you've contributed here before.

And I could even trigger Extra CI :) but workflows still seem to be clogged...

codecov · 2023-03-29T17:30:42Z

Codecov Report

Attention: Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@2a7e5aa). Learn more about missing BASE report.
Report is 80 commits behind head on main.

Files with missing lines	Patch %	Lines
specutils/io/default_loaders/wcs_fits.py	88.88%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1051   +/-   ##
=======================================
  Coverage        ?   70.29%           
=======================================
  Files           ?       64           
  Lines           ?     4410           
  Branches        ?        0           
=======================================
  Hits            ?     3100           
  Misses          ?     1310           
  Partials        ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

to write out fitted cube. Need astropy/specutils#1051 [ci skip] [rtd skip]

pllim · 2023-03-30T14:18:03Z

Not quite there. Looks like flux is written to EXT 0 even when there is mask, which is super confusing. Usually when there are multiple data extensions, EXT 0 is primary. Let's see this example:

import numpy as np
from astropy import units as u
from astropy.io import fits
from astropy.wcs import WCS

from specutils import Spectrum1D

w = WCS(naxis=3)
w.wcs.ctype = 'FREQ', 'DEC--TAN', 'RA---TAN'
w.wcs.set()

sp = Spectrum1D(flux=np.ones((8, 9, 10)) * u.nJy, mask=np.zeros((8, 9, 10), dtype=np.int8), wcs=w)
sp.write("ztmp.fits", format="wcs1d-fits", overwrite=True)
fits.info("ztmp.fits")

With this patch, I get this:

No.    Name      Ver    Type      Cards   Dimensions   Format
  0  FLUX          1 PrimaryHDU      29   (10, 9, 8)   float64
  1  MASK          1 ImageHDU        11   (10, 9, 8)   uint8 (rescales to int8)

But I want this:

No.    Name      Ver    Type      Cards   Dimensions   Format
  0  PRIMARY       1 PrimaryHDU     303   ()
  1  FLUX          1 ImageHDU        29   (10, 9, 8)   float64
  2  MASK          1 ImageHDU        11   (10, 9, 8)   uint8 (rescales to int8)

to write out fitted cube. Need astropy/specutils#1051 [ci skip] [rtd skip]

dhomeier · 2023-03-31T00:32:20Z

Not quite there. Looks like flux is written to EXT 0 even when there is mask, which is super confusing. Usually when there are multiple data extensions, EXT 0 is primary. Let's see this example:

Yes, if you want the flux written to a different extension, you need to specify (e.g.) hdu=1.
The default has always been to write flux to the first available HDU, which for ImageHDU format is the primary HDU.
Mask and uncertainty, if present, are written to the subsequent HDUs per the recent PRs, but flux will be in the same HDU it has always been. I don't quite see how this becomes more confusing when a mask HDU is appended – flux is in HDU 0, whether mask and/or uncertainty are there or not.

pllim · 2023-03-31T13:13:17Z

flux is in HDU 0, whether mask and/or uncertainty are there or not

But this is not how 90% of the data out there are laid out. I have never seen a real life example where the FLUX is still in primary extension if there are ERR or DQ tied to it.

dhomeier · 2023-03-31T20:10:04Z

But this is not how 90% of the data out there are laid out. I have never seen a real life example where the FLUX is still in primary extension if there are ERR or DQ tied to it.

The SDSS spPlate format e.g., with FLUX in the primary HDU and InvVar in the first extension.
SDSS-I/II spSpec also has FLUX in the primary HDU, though with ERR and MASK etc. folded into the same data "cube", and the generic IRAF reader does not even support reading from anything but HDU 0 (but also does not account for any uncertainty or mask data).
APOGEE uses a scheme more similar to what you are describing, but writing flux, StdErr and mask to extensions 1, 2 and 3, but it has its own dedicated loader – however perhaps wcs1d should adhere to that order as well, writing uncertainty before mask?
Bottom line, I don't have any robust percentage numbers on how all spectra out there are distributed, but for the basic image format using the primary HDU seems at least as common as the first extension. In any case the default for writing a flux-only spectrum has been hdu=0 since specutils 1.1, and I don't think it's a good idea to change that now.
OTOH changing the default HDU to 1 depending on the presence of uncertainty and/or mask seems rather more confusing to me (e.g. what should it be if a mask exists, but is skipped in writing by mask_name=None?). But if that is what the majority of users find more logical, so be it...

pllim · 2023-03-31T20:15:12Z

What do the other specutils maintainers think?

should adhere to that order as well, writing uncertainty before mask?

Yes, I think it is common to have the SCI, ERR, DQ in that order.

dhomeier · 2023-04-04T12:51:41Z

Yes, I think it is common to have the SCI, ERR, DQ in that order.

I've changed the order; when updating the tests I realised that the current wcs1d-fits auto-detection also expects the science data in the primary HDU, as it is only probing data structures in HDU 0. So changing this would also break something – either identifying previously written files, or new files (with mask)... In principle identify_wcs1d_fits could also be extended to search through all HDUs for a match, but that would introduce additional complexity (and possibly also performance penalties).

to write out fitted cube. Need astropy/specutils#1051 [ci skip] [rtd skip]

pllim

I had to pass in extra options (hdu=1, flux_name="SCI") to make an existing test pass over at spacetelescope/jdaviz#2094 but it is not the end of the world.

LGTM. Thanks!

rosteen

Code looks fine to me, thanks!

pllim · 2023-04-18T16:01:35Z

Please give me a ping when this ends up in a released version, so I can follow up over at Jdaviz to use this for Cubeviz. Thanks!

to write out fitted cube. This uses astropy/specutils#1051 Update specutils pin.

pllim · 2023-05-10T18:32:27Z

When is the next release for specutils?

* Test warning on unspecified uncertainty type * Store and retrieve boolean masks in wcs1d-fits * Move uncertainty before mask in hdulist

to write out fitted cube. This uses astropy/specutils#1051

dhomeier added 2 commits March 29, 2023 16:44

Test warning on unspecified uncertainty type

650ad1c

Store and retrieve boolean masks in wcs1d-fits

8e8e122

dhomeier added the io label Mar 29, 2023

dhomeier requested a review from pllim March 29, 2023 15:22

dhomeier added Extra CI Run cron in PR and removed Extra CI Run cron in PR labels Mar 29, 2023

pllim approved these changes Mar 29, 2023

View reviewed changes

pllim closed this Mar 29, 2023

pllim reopened this Mar 29, 2023

dhomeier mentioned this pull request Mar 29, 2023

Cubeviz to use wcs1d-fits from specutils to write out fitted cube spacetelescope/jdaviz#2094

Merged

13 tasks

pllim added a commit to pllim/jdaviz that referenced this pull request Mar 30, 2023

Cubeviz to use wcs1d-fits from specutils

619cd58

to write out fitted cube. Need astropy/specutils#1051 [ci skip] [rtd skip]

pllim added a commit to pllim/jdaviz that referenced this pull request Mar 30, 2023

Cubeviz to use wcs1d-fits from specutils

1fba94e

to write out fitted cube. Need astropy/specutils#1051 [ci skip] [rtd skip]

Move uncertainty before mask in hdulist

89634c2

rosteen requested review from keflavich, eteq, nmearl and rosteen April 5, 2023 21:02

pllim added a commit to pllim/jdaviz that referenced this pull request Apr 7, 2023

Cubeviz to use wcs1d-fits from specutils

5c9d516

to write out fitted cube. Need astropy/specutils#1051 [ci skip] [rtd skip]

pllim approved these changes Apr 7, 2023

View reviewed changes

rosteen approved these changes Apr 18, 2023

View reviewed changes

rosteen merged commit b332b87 into astropy:main Apr 18, 2023

pllim added a commit to pllim/jdaviz that referenced this pull request May 10, 2023

Cubeviz to use wcs1d-fits from specutils

d75de7a

to write out fitted cube. This uses astropy/specutils#1051 Update specutils pin.

dhomeier deleted the wcs1d-masked branch May 10, 2023 18:34

pllim added a commit to pllim/jdaviz that referenced this pull request Jul 29, 2024

Cubeviz to use wcs1d-fits from specutils

3b00e22

to write out fitted cube. This uses astropy/specutils#1051

pllim added a commit to pllim/jdaviz that referenced this pull request Jul 30, 2024

Cubeviz to use wcs1d-fits from specutils

9d46233

to write out fitted cube. This uses astropy/specutils#1051

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support write and read of boolean masks in `wcs1d-fits` #1051

Support write and read of boolean masks in `wcs1d-fits` #1051

dhomeier commented Mar 29, 2023

dhomeier commented Mar 29, 2023

rosteen commented Mar 29, 2023

pllim left a comment

pllim commented Mar 29, 2023

pllim commented Mar 29, 2023

dhomeier commented Mar 29, 2023

codecov bot commented Mar 29, 2023 •

edited

Loading

pllim commented Mar 30, 2023

dhomeier commented Mar 31, 2023

pllim commented Mar 31, 2023

dhomeier commented Mar 31, 2023

pllim commented Mar 31, 2023

dhomeier commented Apr 4, 2023

pllim left a comment

rosteen left a comment

pllim commented Apr 18, 2023

pllim commented May 10, 2023

Support write and read of boolean masks in wcs1d-fits #1051

Support write and read of boolean masks in wcs1d-fits #1051

Conversation

dhomeier commented Mar 29, 2023

dhomeier commented Mar 29, 2023

rosteen commented Mar 29, 2023

pllim left a comment

Choose a reason for hiding this comment

pllim commented Mar 29, 2023

pllim commented Mar 29, 2023

dhomeier commented Mar 29, 2023

codecov bot commented Mar 29, 2023 • edited Loading

Codecov Report

pllim commented Mar 30, 2023

dhomeier commented Mar 31, 2023

pllim commented Mar 31, 2023

dhomeier commented Mar 31, 2023

pllim commented Mar 31, 2023

dhomeier commented Apr 4, 2023

pllim left a comment

Choose a reason for hiding this comment

rosteen left a comment

Choose a reason for hiding this comment

pllim commented Apr 18, 2023

pllim commented May 10, 2023

Support write and read of boolean masks in `wcs1d-fits` #1051

Support write and read of boolean masks in `wcs1d-fits` #1051

codecov bot commented Mar 29, 2023 •

edited

Loading